Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ refactoring: unique and is_unique #1111

Merged
merged 19 commits into from
Nov 9, 2021
Merged

C++ refactoring: unique and is_unique #1111

merged 19 commits into from
Nov 9, 2021

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Oct 6, 2021

No description provided.

@ianna ianna marked this pull request as draft October 6, 2021 15:10
@jpivarski
Copy link
Member

In PR #1117 (which will auto-merge soon), all v2 tests are being moved into a new tests/v2 directory, and the "v2" qualifier is being removed from their filenames. Please do that for the tests in this PR, too!

The other directory restructuring that I'll be doing today will be to copy high-level code into src/awkward/v2. At first, it will be commented out, and then I'll slowly uncomment things as they're made to work with v2 only. But since that involves new files, it won't interfere with any of your ongoing work.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@b9a40c8). Click here to learn what that means.
The diff coverage is n/a.

@ianna ianna marked this pull request as ready for review November 8, 2021 16:32
@ianna ianna requested a review from jpivarski November 8, 2021 16:32
@ianna
Copy link
Collaborator Author

ianna commented Nov 8, 2021

@jpivarski - there are a couple of places where for loops should be replaced by newly written kernel functions - they are marked with FIXME. Apart from that I think, it's done.

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

I know this is a lot of work, and I see your FIXMEs of the last issues to fix before it's done. I added to that some choices of exception class: C++ without access to pybind forced us to put a lot of different types of exceptions into only one class, ValueError (std::invalid_argument in C++), but some of the ones labeled below should instead be np.AxisError. We also want to maintain the distinction between user errors (ValueError, TypeError, np.AxisError) and internal programming errors, which are AssertionErrors (RuntimeErrors in Awkward 1, as this is what std::runtime_error produced, and that was our closest C++ equivalent).

One thing that I didn't mention below is to add type-tracer tests after each value test, which I also mentioned to @stormiestsin in #1135 (comment). They can usually be added in a semi-automated way, and they ensure that the code is Dask-ready.

Other than that, thanks for all of this; I can see the intricacy of a lot of these cases!

src/awkward/_v2/contents/content.py Outdated Show resolved Hide resolved
src/awkward/_v2/contents/content.py Outdated Show resolved Hide resolved
src/awkward/_v2/contents/indexedoptionarray.py Outdated Show resolved Hide resolved
src/awkward/_v2/contents/indexedoptionarray.py Outdated Show resolved Hide resolved
src/awkward/_v2/contents/listoffsetarray.py Outdated Show resolved Hide resolved
src/awkward/_v2/contents/listoffsetarray.py Outdated Show resolved Hide resolved
src/awkward/_v2/contents/listoffsetarray.py Outdated Show resolved Hide resolved
src/awkward/_v2/contents/numpyarray.py Outdated Show resolved Hide resolved
tests/v2/test_0404-array-validity-check.py Outdated Show resolved Hide resolved
@ianna ianna merged commit 96cb0f9 into main Nov 9, 2021
@ianna ianna deleted the ianna/unique branch November 9, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants